Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Skip inlining Git LFS placeholders (fix #9714) #9795

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

MajorBreakfast
Copy link
Contributor

Description

Git LFS placeholder files do not contain the actual file content and can therefore not be meaningfully inlined. This PR makes it so that they are automatically excluded from inlining. (Fixes #9714)

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Aug 23, 2022

@bluwy Please have a look at whether this implementation looks like what you had in mind. I'm happy to make adjustments.

I have manually tested the change locally.

@MajorBreakfast
Copy link
Contributor Author

At first glance it looks to me as if the CI is flaky. Can somebody with more insight into this say whether that's indeed the case or whether I need to adapt the code?

@sapphi-red
Copy link
Member

It's failing three times in a row. So I think it's not a flaky fail.
Running pnpm build and pnpm preview at playground/assets might help.

@sapphi-red sapphi-red added the enhancement New feature or request label Aug 23, 2022
@MajorBreakfast
Copy link
Contributor Author

MajorBreakfast commented Aug 23, 2022

@sapphi-red excellent. I'll try that and report back

@MajorBreakfast
Copy link
Contributor Author

The advice with running the assets playground locally helped. 👍 I found the bug

@MajorBreakfast
Copy link
Contributor Author

Just pushed another minor tweak to the docs: "To get inlining, make sure to download the file contents via Git LFS before building." - Just to make clearer what's happening

::: tip Note
If you specify `build.lib`, `build.assetsInlineLimit` will be ignored and assets will always be inlined, regardless of file size.
If you specify `build.lib`, `build.assetsInlineLimit` will be ignored and assets will always be inlined, regardless of file size or being a Git LFS placeholder.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR, but we should find a way to avoid this in lib mode too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, then there's a strong desire to inline everything in lib mode. Hence there are essentially three ways this can behave:

  • Inline the LFS placeholder. Con: The resulting bundle is bricked. The LFS placeholder ins't the actual file content. It's how it behaves today.
  • Skip inlining for the LFS placeholder. Con: Lib mode should bundle everything. So that would be an exception to that rule
  • Display an error to the user. Con: It's an error

In regards to dealing with the situation in this PR there's these options:

  • Keep current behavior and document it
  • Keep current behavior but remove the highlighted sentence from the docs. Then the exact behavior for this edge case is in no longer documented and can be considered undefined behavior. Undefined behavior can be adjusted later without the need of a breaking change
  • Pick another option from three possible behaviors above and document it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could keep the behaviour like in this PR, but if we encounter git lfs in lib build, we can warn about it to download the assets, as it's unlikely someone wants to build for a library that contains unreferenced assets (e.g. git lfs wouldn't work after npm publish)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. I'll see if I can make it produce a warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the code that emits a warning when encountering this edge case. I doubt that anyone will ever see this, though, because it seems rather unusual/unlikely that someone would use Git LFS in a library project :D

patak-dev
patak-dev previously approved these changes Aug 23, 2022
@patak-dev patak-dev requested a review from bluwy August 23, 2022 21:09
bluwy
bluwy previously approved these changes Aug 24, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@MajorBreakfast MajorBreakfast dismissed stale reviews from bluwy and patak-dev via bd14f70 August 24, 2022 09:32
@patak-dev patak-dev merged commit 9c7e43d into vitejs:main Aug 24, 2022
@MajorBreakfast MajorBreakfast deleted the skip-inlining-lfs-placeholders branch August 24, 2022 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Skip asset inlining for Git LFS placeholders
4 participants